feat: add project metadata to project list and view - #693

versionControlBranch can panic on an interface-conversion. The bare type assertion project.PersistenceSettings.(projects.GitPersistenceSettings) panics if the server ever returns IsVersionControlled: true without a Git-typed PersistenceSettings block — in SDK v2.114.1, Project.UnmarshalJSON leaves PersistenceSettings as a nil interface when the field is absent (and the two JSON fields are independent, so nothing guarantees they agree). This predates the PR, but the refactor consolidates it into a helper now called from all three output mappers, so it is a good moment to make it defensive:

Actioned in a1d4f91, with your suggested shape (comma-ok, empty string fallback).

Confirmed it's reachable rather than theoretical: PersistenceSettings carries no validate:"required" tag, so Project.UnmarshalJSON accepts a payload with IsVersionControlled: true and no settings block, leaving the interface nil. The regression test project view does not panic when a version controlled project has no git settings (pkg/cmd/project/view/view_test.go) serves exactly that payload; I reverted versionControlBranch to the bare assertion and ran it, and it panics with interface conversion: interface is nil, not projects.GitPersistenceSettings.

One knock-on, fixed in c72af0e: the "" fallback made basic output print a bare Version control branch: line, which is the same blank-label problem you raised on the project group / lifecycle labels. That label is now skipped when the branch is empty, and the regression test asserts its absence. Table and JSON still get "" for that field — an empty cell and an empty string read fine there, and changing the JSON value to a sentinel would be a schema decision rather than part of a panic fix.

project list -f basic now makes 3 API round trips instead of 1. GetLifecycleMap and GetProjectGroupMap are fetched unconditionally before output.PrintArray, but the Basic mapper only prints p.GetName() — for basic output (common in scripting) the two /all lookups are pure wasted latency and server load. Consider checking the resolved output format first (as PrintArray does via constants.FlagOutputFormat / viper) and skipping the lookups for basic, or lazily populating the maps on first use.

Actioned in 6488338.

PrintArray and PrintResource each had their own copy of the flag-then-viper format resolution, so rather than re-implement it a third time in listRun I extracted it as output.ResolveOutputFormat(cmd) and made both printers call it. listRun now only fetches the two maps when the resolved format isn't basic, so project list -f basic is back to one round trip. Went with the up-front check over lazy population because the maps are read from inside the Table/Json mappers, and lazy fill would mean either a closure with a sync.Once or a nil-map check at each of four call sites for no real gain.

Covered by outputFormat basic lists just names, without the name lookups in pkg/cmd/project/list/list_test.go: it queues only root, space and projects/all, and because the mock server has no response waiting for a fourth request the test deadlocks (and the package times out) if the lookups come back. That's how I confirmed the assertion actually bites.

Deliberately not changed: project view still does its two single-ID lookups unconditionally, because its basic output does print the project group and lifecycle names, so there's nothing to skip there.

This duplicates getLifecycleMap in pkg/cmd/channel/list/list.go (line 151) byte-for-byte. Since this PR promotes the pattern to a shared helper, consider migrating channel list to call it too (or placing the helper in a neutral package outside pkg/cmd/project/ so a channel command importing it does not read oddly). Two identical copies will drift — e.g. if pagination or error reporting ever changes.

Actioned in e3f4330, taking the second of your two options.

GetLifecycleMap, GetProjectGroupMap, GetLifecycleName, GetProjectGroupName and DisplayName moved to a new neutral pkg/lookups package, and pkg/cmd/channel/list/getLifecycleMap is deleted in favour of lookups.GetLifecycleMap. That avoids a channel command importing a project command's shared package, which was the part that read oddly. pkg/cmd/project/shared keeps TenantedDeploymentMode, since that is genuinely project specific.

No new tests — this is a move plus one call-site swap, and the behaviour is covered by the existing pkg/cmd/channel/list and pkg/cmd/project/{list,view} suites, which pass.

Worth noting for later: the shared copy inherits the channel version's blind spot rather than fixing it. Lifecycles.GetAll() / ProjectGroups.GetAll() swallow the error and return an empty map, so a 403 or a transport failure is indistinguishable from "no lifecycles exist" and the caller silently shows IDs. That's now one place to fix instead of two, which was your point, but I haven't changed the semantics in this PR.

Minor: empty ID prints a blank labeled line. GetLifecycleName/GetProjectGroupName explicitly guard id == "", but when that case fires, DisplayName("", "") returns "", so basic output prints Project group: / Lifecycle: with nothing after the label (and the table shows an empty cell). If an empty ID is reachable, a placeholder like Unknown (or skipping the line) would read better.

Actioned in e74c28d — the label is skipped rather than printed with nothing after it — but with a correction to the "if an empty ID is reachable" premise: for these two fields it isn't, at least not through project view.

Project.UnmarshalJSON in the SDK validates the decoded payload with validator.Struct, and both fields are tagged validate:"required" (v2.114.1, pkg/projects/project.go: LifecycleID line 94, ProjectGroupID line 96). So a project response with an empty LifecycleId or ProjectGroupId never deserialises. I tried to write a test for it and watched Projects.GetByIdentifier swallow the unmarshal failure and fall back to GET /api/Spaces-1/projects?partialName=Projects-22 — the formatter is never reached. So the guard is pure defence in depth, and I've left it untested rather than build a fake search-fallback response to exercise dead code.

I picked skipping the line over an Unknown placeholder because the non-empty-ID case already degrades gracefully (DisplayName falls back to the ID — covered by project view falls back to IDs when the lookups fail (basic)), so the only way to reach a blank label is a payload the SDK rejects.

The blank label that is reachable is the one right below: versionControlBranch returns "" for a version-controlled project with no Git settings, which printed Version control branch: with nothing after it. Fixed the same way in c72af0e, and that one has a test.

Pre-existing quirk this refactor keeps: browser.OpenURL lives inside the Basic formatter, so octopus project view X --web -f table or -f json silently never opens the browser — the flag only works with the default basic format. Worth hoisting the opts.flags.Web.Value check into viewRun so --web behaves the same for every output format.

Actioned in 67e3cc6, in this PR rather than a follow-up — but flagging it as the one change here that's a judgement call, since you're right that the quirk is pre-existing.

Why I kept it in: the fix is the flag check moving into viewRun, and viewRun plus that exact block of the basic formatter are already rewritten by this PR (the webUrl extraction touches the same lines). Splitting it out would mean a follow-up PR whose whole diff conflicts with this one for no isolation benefit. Easy to pull back out if you'd rather keep the PR strictly to metadata — it's a single commit at the tip, so git revert 67e3cc6 here and re-land it separately is clean.

Two things to be aware of in the new behaviour:

• --web now opens the browser before the output is written, where previously (basic only) it opened after. Nothing depends on the ordering, but it is a visible difference if you're watching a terminal.
• The browser.OpenURL error is still discarded, now as an explicit _ =. Reporting it would change project view --web from "prints the project, quietly fails to open a browser" to something that can fail the command, which felt out of scope. Happy to surface it to stderr instead if you'd prefer.

No test: asserting on browser.OpenURL needs an injection seam the command doesn't have, and adding one for this seemed disproportionate. So this one is verified by reading only — I have not run project view --web -f json against a real server.

There's a lot of detail in this comment, and it's not exactly clear what's important about it. I'd suggest simplifying or removing.

Trimmed in cdae0ec, down to the one thing the code cannot say:

// Basic output only prints names, so don't pay for the lookups there
var lifecycleIdToNameMap, projectGroupIdToNameMap map[string]string

The two lines I dropped were both restating the code — that it is two calls for the whole list rather than one per project, and that the lookups are best-effort. The first is visible from the call site and the second now lives on the lookup functions' own doc comments, which is where you asked for it.

Applied in 4b84883, along with the other three doc comments and the local it is assigned to.

Applied in 4b84883.

Applied in 4b84883.

Applied in 4b84883.

Applied in 4b84883. Also renamed the four read sites in the Json and Table mappers, and pkg/cmd/channel/list picks up the renamed GetLifecycleIdToNameMap — it is the only other caller.

This is the one suggestion I have not taken literally — I think it is a find/replace artifact rather than the name you meant.

These two locals hold a single resolved name, not a map, so lifecycleIdToNameName does not describe them (and GetLifecycleName / GetProjectGroupName keep their names in your other four suggestions, so the call is unchanged either way). I have left them as:

lifecycleName := lookups.GetLifecycleName(opts.Client, project.LifecycleID)
projectGroupName := lookups.GetProjectGroupName(opts.Client, project.ProjectGroupID)

The ID-to-name naming you asked for is applied where there actually is a map — lifecycleIdToNameMap / projectGroupIdToNameMap in project list, in 4b84883. Say the word if there is a different name you had in mind for these two and I will take it.

The comment above them is gone in cdae0ec, per your other note.

This comment isn't valuable.

Removed in cdae0ec.

Applied in cdae0ec, verbatim.

Shortened the sibling comment below it the same way while I was there — it was two lines making the same point about the version control branch label, now one.